refactor: align List tokens, spacing and typography with MD3 - #5041
refactor: align List tokens, spacing and typography with MD3#5041adam-sajko wants to merge 3 commits into
Conversation
MikitasK
left a comment
MikitasK
left a comment
There was a problem hiding this comment.
good job 👏 overall, this refactoring looks solid 👍
just a few thing to consider before merge:
| <Text | ||
| variant="bodyMedium" | ||
| selectable={false} | ||
| numberOfLines={descriptionNumberOfLines} | ||
| ellipsizeMode={descriptionEllipsizeMode} | ||
| style={[ | ||
| styles.description, | ||
| { color: descriptionColor }, | ||
| descriptionStyle, | ||
| ]} | ||
| style={[{ color: descriptionColor }, descriptionStyle]} | ||
| onTextLayout={onDescriptionTextLayout} | ||
| maxFontSizeMultiplier={descriptionMaxFontSizeMultiplier} | ||
| > |
There was a problem hiding this comment.
what about passing resolved theme to these Text components?
<Text theme={theme} variant="bodyMedium" ... >
in current implementation component-level font overrides are used for render-prop titles & descriptions, but plain text still reads typography from the surrounding provider
| <Text | ||
| variant="bodyLarge" | ||
| selectable={false} | ||
| ellipsizeMode={titleEllipsizeMode} | ||
| numberOfLines={titleNumberOfLines} | ||
| style={[styles.title, { color: titleColor }, titleStyle]} | ||
| style={[{ color: titleColor }, titleStyle]} | ||
| maxFontSizeMultiplier={titleMaxFontSizeMultiplier} | ||
| > | ||
| {title} | ||
| </Text> |
| @@ -285,10 +290,10 @@ const ListAccordion = ({ | |||
| </Text> | |||
| {description ? ( | |||
| <Text | |||
| variant="bodyMedium" | |||
| selectable={false} | |||
| numberOfLines={descriptionNumberOfLines} | |||
| style={[ | |||
| styles.description, | |||
| { | |||
| color: descriptionColor, | |||
| }, | |||
| @@ -301,20 +306,15 @@ const ListAccordion = ({ | |||
| </Text> | |||
| style={[styles.container, style]} | ||
| style={[ | ||
| styles.container, | ||
| description ? styles.containerTwoLine : styles.containerOneLine, |
There was a problem hiding this comment.
what about descriptions with 2+ lines? they currently keep 14dp 2-line padding, resulting in 92dp item instead of MD3 88dp minimum (source)
so can we use 12dp vertical padding (multilineVerticalPadding: 12) whenever lines.length >= 2?
| description ? styles.containerTwoLine : styles.containerOneLine, | |
| description | |
| ? isDescriptionMultiline | |
| ? styles.containerMultiline | |
| : styles.containerTwoLine | |
| : styles.containerOneLine |
There was a problem hiding this comment.
Done, also corrected end inset 24 -> 16 to match current MD3/Compose
Just one caveat: onTextLayout isn’t implemented in react-native-web, so this tier only applies on native. That’s pre-existing (alignToTop already does nothing on web before)
| style={[styles.container, style]} | ||
| style={[ | ||
| styles.container, | ||
| description ? styles.containerTwoLine : styles.containerOneLine, |
2 participants
Motivation
List.Item and List.Accordion now follow the MD3 list spec for padding, type, and colour instead of hardcoded sizes and the old expanded primary tint.
Visual changes
Related issue
Related to #4976
Test plan